Use modern v8::Platform worker threads APIs. #69

Chubab · 2018-05-09T16:23:33Z

Precursor to removing deprecated APIs on the v8 side @
https://chromium-review.googlesource.com/c/v8/v8/+/1045310

Nomenclature has been updated from "background threads" to "worker threads".

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Precursor to removing deprecated APIs on the v8 side @ https://chromium-review.googlesource.com/c/v8/v8/+/1045310

addaleax · 2018-05-09T16:25:49Z

src/node_platform.cc

-    // be available anyway, though.
-    background_task_runner_->BlockingDrain();
+    // Worker tasks aren't associated with an Isolate.
+    worker_thread_task_runner_->BlockingDrain();


Is there any reason for updating the comment?

Wanted to remove
"In the long run, that functionality is probably going to be available anyway, though."
I can leave the rest if you think it's better than my shorter version.

You can feel free to remove it, but that sentence was more or less a reminder to myself that we might want this, so I was curious if there’s anything inherently wrong with the idea?

There should be worker threads for each core regardless of the number of isolates. Isolates are bound to a single task runner (usually mapped to a single thread), but worker threads are shared so we removed the Isolate* parameter (both Chrome and node didn't use it).

Just for context, the original idea was that when an Isolate is being removed (e.g. one for a WebWorker-style thread), we want the tasks associated with it to be drained first before disposing it; so what we’d have to do is to is to somehow keep track of which tasks were created for which Isolate, even the background tasks. I might be totally off about this. :)

Tasks posted to workers should either not use state bound to the Isolate or be cancellable.

Isolate::DeInit() makes sure to wait for cancelled tasks to be flagged as cancelled or be done running via CancelAndWait() @ https://cs.chromium.org/chromium/src/v8/src/isolate.cc?type=cs&q=file:isolate.cc+CancelAndWait&sq=package:chromium

As such, there's no need to drain tasks before getting rid of an Isolate, I think. It would be a good idea to mention this specifically in v8::Platform, make sure it's true for existing callers, and remove the draining logic in node IMO (but, to be clear, I will not do this -- just looking to clean up the old APIs before transitioning).

Thanks, this was helpful!

but, to be clear, I will not do this

I can take care of at least documenting it. :)

Indeed, at the moment every task posted by V8 should be cancelable, so V8 can kind of clean up the task queue by itself when the isolate is tearing down. The reason is that tasks should not own the memory they operate on. This invariant avoids synchronization issues between isolate shutdown and task destruction. Therefore the isolate (directly or indirectly) owns the memory of the task, which is why the task has to be cancelled before the isolate goes away.

I will add some documentation for this in V8 as well.

addaleax · 2018-05-09T16:26:57Z

src/node_platform.cc

-void BackgroundTaskRunner::PostDelayedTask(std::unique_ptr<v8::Task> task,
-                                           double delay_in_seconds) {
+void WorkerThreadsTaskRunner::PostDelayedTask(std::unique_ptr<v8::Task> task,
+                                              double delay_in_seconds) {
  UNREACHABLE();
 }


Just to verify: Even though CallDelayedOnWorkerThread() is being added to the API, this is still unreachable code atm?

Indeed. This doesn't change the previous logic. Having this be UNREACHABLE() was likely incorrect before and still is (but I'm not going to change that).

In practice the only v8 caller is devtools which I assume doesn't trigger in node so node has been getting away with this UNREACHABLE for now...

It's unfortunate that CallDelayedOnWorkerThread() was used in V8 for the first time just before we were about to remove the API. I guess we can implement it the same as for foreground tasks if we ever have to.

hashseed · 2018-05-09T16:27:56Z

I'm on vacation right now, but will merge after getting some opinions.

gahaas

LGTM

gahaas · 2018-05-09T20:51:34Z

src/node_platform.h

@@ -148,7 +146,7 @@ class NodePlatform : public MultiIsolatePlatform {
                     std::shared_ptr<PerIsolatePlatformData>> per_isolate_;

  std::unique_ptr<v8::TracingController> tracing_controller_;
-  std::shared_ptr<BackgroundTaskRunner> background_task_runner_;
+  std::shared_ptr<WorkerThreadsTaskRunner> worker_thread_task_runner_;


I think this can be a shared_ptr now.

What do you mean? It is an std::shared_ptr..?

Ah, sorry. I meant, can it be a unique_ptr now?

hashseed · 2018-05-09T21:18:41Z

Will land once comments are addressed. Please squash additional changes to the existing commit, thanks.

Chubab · 2018-05-10T14:13:24Z

@hashseed : Unless I missed some, I believe all of the comments were discussions that didn't lead to requiring a code update. Except for @gahaas 's last comment about shared_ptr which I don't understand...

I think this can be landed as-is. Thanks!

…PIs pure virtual. Also fixup some implementations that were lagging behind per the lack of pure virtual not having enforced everything yet. Also fixed recently introduced PredictablePlatform::CallDelayedOnWorkerThread() to ignore delayed tasks after realizing the intent is to intercept worker tasks instead of sending them to |platform_|. Node.js migrated off these APIs @ v8/node#69 [email protected], [email protected] Bug: chromium:817421 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I92171f213b5fc64ab1f21e8eec72738f5ce228bd Reviewed-on: https://chromium-review.googlesource.com/1045310 Commit-Queue: Gabriel Charette <[email protected]> Reviewed-by: Ulan Degenbaev <[email protected]> Reviewed-by: Andreas Haas <[email protected]> Cr-Commit-Position: refs/heads/master@{#53223}

Use modern v8::Platform worker threads APIs.

847ebc0

Precursor to removing deprecated APIs on the v8 side @ https://chromium-review.googlesource.com/c/v8/v8/+/1045310

addaleax approved these changes May 9, 2018

View reviewed changes

gahaas approved these changes May 9, 2018

View reviewed changes

hashseed merged commit 2230a51 into v8:vee-eight-lkgr May 10, 2018

addaleax mentioned this pull request May 18, 2018

CallOnBackgroundThread removed from v8-platform nodejs/node-v8#62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use modern v8::Platform worker threads APIs. #69

Use modern v8::Platform worker threads APIs. #69

Chubab commented May 9, 2018

addaleax May 9, 2018

Chubab May 9, 2018

addaleax May 9, 2018

Chubab May 9, 2018

addaleax May 9, 2018

Chubab May 9, 2018

addaleax May 9, 2018

gahaas May 9, 2018 •

edited

Loading

addaleax May 9, 2018

Chubab May 9, 2018

gahaas May 9, 2018 •

edited

Loading

hashseed commented May 9, 2018

gahaas left a comment

gahaas May 9, 2018

Chubab May 10, 2018

gahaas May 11, 2018

hashseed commented May 9, 2018

Chubab commented May 10, 2018

Use modern v8::Platform worker threads APIs. #69

Use modern v8::Platform worker threads APIs. #69

Conversation

Chubab commented May 9, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gahaas May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gahaas May 9, 2018 • edited Loading

Choose a reason for hiding this comment

hashseed commented May 9, 2018

gahaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashseed commented May 9, 2018

Chubab commented May 10, 2018

gahaas May 9, 2018 •

edited

Loading

gahaas May 9, 2018 •

edited

Loading